Skip to content

Conversation

Patrick-Erichsen
Copy link
Collaborator

@Patrick-Erichsen Patrick-Erichsen commented Oct 16, 2025

closes CON-4324 CON-3616 CON-4465

Summary by cubic

Allow reading and listing files outside the IDE workspace with explicit permission. Adds robust path resolution for absolute, tilde (~/), and file:// paths, and enforces stricter access policies for non-workspace paths.

  • New Features
    • Added pathResolver to normalize user-provided paths and detect workspace boundaries; supports relative, absolute, tilde, and file:// URIs (with tests).
    • Introduced evaluateFileAccessPolicy: files outside the workspace always require permission; integrated via preprocessArgs + evaluateToolCallPolicy for ls, readFile, readFileRange, and viewSubdirectory.
    • Core now supports tool arg preprocessing and passes processed args to policy evaluation; terminal command display uses processed args.
    • Updated tool implementations to use resolveInputPath with clearer errors and consistent display paths; added a CLI path resolver for local runs.

@Patrick-Erichsen Patrick-Erichsen marked this pull request as ready for review October 16, 2025 22:57
@Patrick-Erichsen Patrick-Erichsen requested a review from a team as a code owner October 16, 2025 22:57
@Patrick-Erichsen Patrick-Erichsen requested review from tingwai and removed request for a team October 16, 2025 22:57
@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Oct 16, 2025
@Patrick-Erichsen Patrick-Erichsen requested review from RomneyDa and removed request for tingwai October 16, 2025 22:58
- Combined resolveInputPath approach with new ContinueError system
- Updated error handling to use ContinueError with appropriate error reasons
- Maintained improved error messages from pe/read-file-errs branch
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 issues found across 13 files

Prompt for AI agents (all 5 issues)

Understand the root cause of the following 5 issues and fix them.


<file name="core/tools/implementations/viewSubdirectory.ts">

<violation number="1" location="core/tools/implementations/viewSubdirectory.ts:11">
resolveInputPath returns a truthy result even when the absolute directory does not exist, so this branch no longer throws DirectoryNotFound and generateRepoMap later fails while walking the missing path.</violation>
</file>

<file name="extensions/cli/src/util/pathResolver.ts">

<violation number="1" location="extensions/cli/src/util/pathResolver.ts:15">
Home-relative paths that use &quot;~\&quot; (common on Windows shells) are not expanded, so resolveInputPath incorrectly rejects valid Windows home paths.</violation>
</file>

<file name="core/util/pathResolver.ts">

<violation number="1" location="core/util/pathResolver.ts:26">
Workspace containment currently matches on raw prefix, so paths such as `/workspace-other/file` are incorrectly classified as inside `/workspace`, which can bypass the outside-workspace permission gate. Please ensure the comparison enforces real directory boundaries.</violation>
</file>

<file name="core/util/pathResolver.test.ts">

<violation number="1" location="core/util/pathResolver.test.ts:80">
The expected URI is constructed with path.join, which yields backslashes and omits the third slash on Windows, so this assertion fails on Windows even though resolveInputPath returns the correct normalized URI.</violation>

<violation number="2" location="core/util/pathResolver.test.ts:201">
This assertion hardcodes a POSIX path; on Windows normalizeDisplayPath yields `~\Documents\file.txt`, so the test fails despite the implementation being correct on that platform.</violation>
</file>

React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed changes from recent commits (found 1 issue).

1 issue found across 5 files

Prompt for AI agents (all 1 issues)

Understand the root cause of the following 1 issues and fix them.


<file name="core/util/pathResolver.test.ts">

<violation number="1" location="core/util/pathResolver.test.ts:38">
The new findUriInDirs mock treats file:///workspace-subdir/... as within the workspace because it only checks uri.startsWith(dir). Please ensure the mock only matches when the URI actually sits under the workspace path, mirroring the real helper.</violation>
</file>

React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 4 files

Patrick-Erichsen and others added 2 commits October 17, 2025 10:19
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed changes from recent commits (found 1 issue).

1 issue found across 3 files

Prompt for AI agents (all 1 issues)

Understand the root cause of the following 1 issues and fix them.


<file name="core/tools/implementations/viewSubdirectory.vitest.ts">

<violation number="1" location="core/tools/implementations/viewSubdirectory.vitest.ts:7">
The mocked extras.ide object is missing getWorkspaceDirs, so resolveInputPath throws a TypeError before viewSubdirectoryImpl can raise ContinueError, causing the test to fail.</violation>
</file>

React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Oct 17, 2025
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files


// Expand tilde paths
let expandedPath = trimmedPath;
if (trimmedPath.startsWith("~/")) {
Copy link
Collaborator

@RomneyDa RomneyDa Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noting no tilde support for windows, probably fine since tilde is usually unix, but e.g. ~\this\that would be missed here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont think that is a valid windows path

Windows doesn't interpret ~ as the home directory like Unix/Linux systems do. Valid Windows paths typically start with:
  - A drive letter: C:\this\that
  - A UNC path: \\server\share\this\that
  - A relative path: this\that or .\this\that

  To reference the user's home directory in Windows, you'd use environment variables like %USERPROFILE%\this\that or %HOMEPATH%\this\that.


if (isAbsolute) {
// For Windows network paths, handle specially
if (expandedPath.startsWith("\\\\")) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check is duplicate, can move windows network path handling outside/above other absolute paths

// For Windows network paths, handle specially
if (expandedPath.startsWith("\\\\")) {
const networkPath = expandedPath.replace(/\\/g, "/");
const uri = "file:" + networkPath; // file://server/share format
Copy link
Collaborator

@RomneyDa RomneyDa Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note networkPath is not normalized to URI path style, probably fine but could add URI.normalize here

@github-project-automation github-project-automation bot moved this from Todo to In Progress in Issues and PRs Oct 17, 2025
@Patrick-Erichsen
Copy link
Collaborator Author

Patrick-Erichsen commented Oct 17, 2025

After pulling in untildify and normalize-path
Screenshot 2025-10-17 at 2 31 58 PM

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed changes from recent commits (found 2 issues).

2 issues found across 5 files

Prompt for AI agents (all 2 issues)

Understand the root cause of the following 2 issues and fix them.


<file name="core/tools/implementations/viewSubdirectory.vitest.ts">

<violation number="1" location="core/tools/implementations/viewSubdirectory.vitest.ts:9">
This test no longer covers the resolveInputPath-null scenario because the mocked extras cause fileExists to trigger the failure instead, so it will not detect regressions in that branch.</violation>
</file>

<file name="core/util/pathResolver.ts">

<violation number="1" location="core/util/pathResolver.ts:59">
UNC file paths are no longer converted correctly: pathToFileURL on POSIX hosts turns \\server\share paths into URIs like file:///mnt/workspace/%5Cserversharedir, breaking access to network shares.</violation>
</file>

React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 3 files

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 9 files

RomneyDa
RomneyDa previously approved these changes Oct 17, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Oct 17, 2025
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 3 files

@Patrick-Erichsen
Copy link
Collaborator Author

Note that I added a .skip to DocsCrawler.test.ts - it flaked 3+ times on me and this feature is deprecated so I think we should just skip the test rather than try to resolve the flake.

Copy link
Collaborator

@RomneyDa RomneyDa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RomneyDa RomneyDa merged commit 1d15c7c into main Oct 20, 2025
79 of 85 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Issues and PRs Oct 20, 2025
@RomneyDa RomneyDa deleted the pe/read-file-errs branch October 20, 2025 17:50
@github-actions github-actions bot locked and limited conversation to collaborators Oct 20, 2025
@github-actions github-actions bot added the tier 1 Big feature that took multiple weeks to launch and represents a big milestone for the product label Oct 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files. tier 1 Big feature that took multiple weeks to launch and represents a big milestone for the product

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants